fix(db): never raise from the connect handler, and scale the budget test right - #124
Merged
Conversation
…est right Two failures from the post-merge run of 12842d2, one real and one mine. test-windows: tests/test_highlevel.py's corrupt-database test passed and then failed its own temporary-directory cleanup with WinError 32, because recording.db was still open. get_engine asks each new connection for its journal mode so it can match the synchronous setting to it. A corrupt file cannot answer, and the exception was raised inside the connect handler, which leaves the new connection outside the pool that would have closed it: engine.dispose() cannot reach it and the file stays open. macOS and Linux delete an open file, so only Windows reported it. Never raise from that handler. It is a tuning step, not a correctness one, and a file that cannot answer it has to fail on the caller's own statement instead, where the engine still owns the connection. A new test asserts the corrupt-file path leaves no open handle, and fails against the unguarded handler. test-macos: the budget test measured 2.05s, then 2.22s, against declared worst cases of 2.0s and 2.2s. The helper is right; the test's scaling was wrong. An attempt costs its busy timeout plus a fixed overhead that does not shrink with it, so a ceiling of 0.2s against a 0.05s timeout is mostly spent on overhead and one slow attempt overruns. Production keeps the ceiling several times the timeout, so scale the test the same way: 0.25s timeout, 1.0s ceiling, 5.0s budget. Eight local runs measure 4.65-4.74s against a 6.0s worst case, where the old scaling had 0.02s of margin. Verified again with only the old retry policy restored: both regression tests fail, the other 14 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two failures from the post-merge run of
12842d28(#123). One is a real defect I introduced in #122; the other is my test's scaling.test-windows — a real handle leak
tests/test_highlevel.py::TestCaptureEdgeCases::test_capture_load_corrupt_dbpassed and then failed its own temporary-directory cleanup:get_engineasks each new connection for its journal mode so it can matchsynchronousto it. A corrupt file cannot answer, and that exception was raised inside the connect handler — which leaves the new connection outside the pool that would have closed it.engine.dispose()cannot reach it, and the file stays open. macOS and Linux happily delete an open file, so only Windows reported it.Never raise from that handler. It is a tuning step, not a correctness one. A file that cannot answer it must fail on the caller's own statement instead, where the engine still owns the connection and disposes it.
test_a_corrupt_capture_database_leaves_no_open_handlepins this, and fails against the unguarded handler.test-macos — my test's scaling, not the helper
The budget test measured 2.05s, then 2.22s, against declared worst cases of 2.0s and 2.2s.
An attempt costs its busy timeout plus a fixed overhead that does not shrink with it. A 0.2s ceiling against a 0.05s busy timeout is mostly that overhead, so one slow attempt overruns. Production deliberately keeps the ceiling several times the timeout (2.0s vs 0.5s); the test did not.
Scaled to match: 0.25s timeout, 1.0s ceiling, 5.0s budget.
No change to the retry helper in either case.
Still discriminating
Verified again with only the old retry policy restored:
test_the_total_wait_never_runs_past_the_declared_budgetandtest_concurrent_writers_all_survive_a_busy_write_lockboth fail, the other 14 pass.Full suite: 722 passed, 33 skipped.
ruffclean.